Skip to content

feat: enable producing to event bus via settings - #249

Merged
navinkarkera merged 2 commits into
openedx:mainfrom
open-craft:navin/configurable-handler
Aug 28, 2023
Merged

feat: enable producing to event bus via settings#249
navinkarkera merged 2 commits into
openedx:mainfrom
open-craft:navin/configurable-handler

Conversation

@navinkarkera

@navinkarkera navinkarkera commented Jul 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description: Implements ADR merged in #224 .

ISSUE: #210

Testing instructions:

  • Clone this repository and checkout this PR.
  • Update stub implementation of producer based on below patch
diff --git a/openedx_events/event_bus/__init__.py b/openedx_events/event_bus/__init__.py
index 582effe..a4bf597 100644
--- a/openedx_events/event_bus/__init__.py
+++ b/openedx_events/event_bus/__init__.py
@@ -100,6 +100,11 @@ class NoEventBusProducer(EventBusProducer):
             event_metadata: EventsMetadata,
     ) -> None:
         """Do nothing."""
+        print("++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++")
+        print(f"""NoEventBusProducer#send signal: {signal}""")
+        print(f"""NoEventBusProducer#send topic: {topic}""")
+        print(f"""NoEventBusProducer#send event_key_field: {event_key_field}""")
+        print(f"""NoEventBusProducer#send event_data: {event_data}""")
+        print(f"""NoEventBusProducer#send event_metadata: {event_metadata}""")
+        print("++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++")
  • Install requirements locally using make requirements.
  • Open a django shell using python manage.py shell.
  • Run below snippet and verify that handlers print correct data.
from openedx_events.content_authoring.signals import XBLOCK_DELETED, XBLOCK_PUBLISHED, XBLOCK_DUPLICATED
from openedx_events.content_authoring.data import XBlockData, DuplicatedXBlockData
data = XBlockData(usage_key="block-v1:edx+DemoX+Demo_course+type@video+block@UaEBjyMjcLW65gaTXggB93WmvoxGAJa0JeHRrDThk", block_type="video")
dup = DuplicatedXBlockData(usage_key="block-v1:edx+DemoX+Demo_course+type@video+block@UaEBjyMjcLW65gaTXggB93WmvoxGAJa0JeHRrDThk", source_usage_key="block-v1:edx+DemoX+Demo_course+type@video+block@UaEBjyMjcLW65gaTXggB93WmvoxGAJa0JeHRrDThk", block_type="video")
XBLOCK_DELETED.send_event(xblock_info=data)
XBLOCK_PUBLISHED.send_event(xblock_info=data)
XBLOCK_DUPLICATED.send_event(xblock_info=dup)
  • Make sure that only the events configured in EVENT_BUS_PRODUCER_CONFIG are printed by the stub implementation of event bus producer.

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 19, 2023
@openedx-webhooks

openedx-webhooks commented Jul 19, 2023 •
edited
Loading

Copy link
Copy Markdown

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch 3 times, most recently from 14d23f1 to 10dabba Compare July 19, 2023 14:03
@navinkarkera
navinkarkera marked this pull request as ready for review July 19, 2023 14:03
@navinkarkera
navinkarkera requested a review from a team July 19, 2023 14:03
@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch from 10dabba to e01833b Compare July 20, 2023 10:14
tecoholic
tecoholic approved these changes Jul 24, 2023
View reviewed changes

@tecoholic tecoholic left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navinkarkera 👍 LGTM

  • I tested this: Followed the testing instructions and verified that signals are being sent as per the configuration.
  • I read through the code
  • I checked for accessibility issues - NA
  • Includes documentation

A couple of points I noticed:

  • fastavro 1.8.0 seems to have some build issues (it failed to build for me on Apple ARM64) and they seems to have fixed it in version 1.8.1. I was able to test it after changing requirements/dev.txt to use fastavro==1.8.1. Maybe it's worth updating it.
  • I added a suggestion about using Pydantic for config validation. I haven't considered the cost of adding an additional dependency, but, it might be worth considering and it definitely not a blocker here.

Comment thread docs/how-tos/adding-events-to-event-bus.rst Outdated
Comment thread docs/how-tos/adding-events-to-event-bus.rst Outdated
Comment thread openedx_events/apps.py Outdated
Comment thread openedx_events/apps.py Outdated
Comment thread openedx_events/apps.py
@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch 2 times, most recently from c36b2a8 to b991f04 Compare July 24, 2023 13:55
This was referenced Jul 28, 2023
Closed
Closed
@robrap robrap added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jul 31, 2023
@robrap

robrap commented Jul 31, 2023

Copy link
Copy Markdown
Contributor

@navinkarkera: I added waiting for eng review for my team (arch-bom). I'm trying this process out. Let me know if this ends up being a process problem. Thanks.

rgraber
rgraber approved these changes Aug 1, 2023
View reviewed changes

@rgraber rgraber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but nothing blocking.

Comment thread docs/how-tos/adding-events-to-event-bus.rst Outdated
Comment thread openedx_events/apps.py
Comment thread openedx_events/apps.py Outdated
Comment thread openedx_events/apps.py
@rgraber rgraber mentioned this pull request Aug 2, 2023
Closed
3 tasks
@rgraber rgraber added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Aug 8, 2023
@rgraber

rgraber commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

@navinkarkera I think this is ready to merge. I had nits but you can choose to address them or not, they're not blocking.

@mphilbrick211

mphilbrick211 commented Aug 22, 2023

Copy link
Copy Markdown

Hi @navinkarkera! Just flagging that some branch conflicts have popped up.

@robrap

robrap commented Aug 22, 2023

Copy link
Copy Markdown
Contributor

@mphilbrick211: We heard Navin may be on vacation, so we'll hear back when we hear back.

@mphilbrick211

mphilbrick211 commented Aug 22, 2023

Copy link
Copy Markdown

@mphilbrick211: We heard Navin may be on vacation, so we'll hear back when we hear back.

Yes, thank you, @robrap! I forgot he was away.

@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch 2 times, most recently from ffc3d29 to 86b74ae Compare August 28, 2023 10:38
@navinkarkera
feat: enable producing to event bus via settings
fa2f5da 
docs: update adding event to bus document

chore: fix lint issues

fix: docs and type checking

Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>

docs: update events how to

fix: Update openedx_events/apps.py

Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>

refactor: catch bad event type and log
@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch 2 times, most recently from 2c2a3bc to e6d8820 Compare August 28, 2023 10:53
@navinkarkera
navinkarkera force-pushed the navin/configurable-handler branch from e6d8820 to 4b786f3 Compare August 28, 2023 10:57
@navinkarkera
navinkarkera merged commit 4ed4567 into openedx:main Aug 28, 2023
@openedx-webhooks

openedx-webhooks commented Aug 28, 2023

Copy link
Copy Markdown

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@navinkarkera
navinkarkera deleted the navin/configurable-handler branch August 28, 2023 17:23
This was referenced Sep 14, 2023
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@tecoholic tecoholic tecoholic approved these changes
@rgraber rgraber rgraber approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@navinkarkera @openedx-webhooks @robrap @rgraber @mphilbrick211 @tecoholic